Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor cleanups and some new example programs. #67

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

phbcanada
Copy link

The threaded streaming example program wasn't working so I fixed it.
The new "set_ftp" example is useful for me since I use FTP to record all my camera motion videos.
The "snap" example is useful for grabbing snapshots from each camera.
I use hostnames instead of IP addresses for my cameras. Just put the IP addresses in /etc/hosts.

Loosen restrictions.
Improvements to share common code.
Fix threaded version to open window in main thread.
Forgot to commit with previous.
New method: stop_stream.
New camera method: is_logged_in
New example program to set ftp parameters.
get_info calls all the info methods.
snap will snap a single frame image.
Getting occasional garbage back from the cameras which causes the json conversion to crap out. Print some info when this happens.
Copy link
Member

@Benehiko Benehiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @phbcanada

Thank you for the contribution! This looks quite good and we should easily be able to merge it :)

Just one question about the version specifier inside the setup.py file.

Comment on lines +35 to 38
'numpy>=1.19.4',
'opencv-python>=4.4.0.46',
'Pillow>=8.0.1',
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I agree with the inclusive ordered comparison >=. Minor versions could introduce breaking changes at some point. I would rather stick with a specific version and in this case update the specific version manually to a later release.

https://peps.python.org/pep-0440/#version-specifiers

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The install failed on one of my systems so this was the easy fix. Not a big issue for me.

@Benehiko
Copy link
Member

Could we also use conventional commits on the PR title?

I know this repository doesn't have any real guidelines on this and in the past it's been quite lax, but I think it is good practice.

you can change the title to something like fix: minor cleanups and new examples.

https://www.conventionalcommits.org/en/v1.0.0/#summary

@phbcanada
Copy link
Author

I haven't used git/github for several years. Not really up on current conventions. Sorry.

@Benehiko
Copy link
Member

I haven't used git/github for several years. Not really up on current conventions. Sorry.

np :)

The GetAbility command returns a property named "scheduleVersion" which indicates which API calls should be used. The new ones have a "V20" tacked onto the command name.
@tinito
Copy link

tinito commented Oct 11, 2023

Any chances to get this merged? The set_network_ftp() method would be really helpful.

@themoosman
Copy link
Contributor

Any chances to get this merged? The set_network_ftp() method would be really helpful.

There looks to be a simple requested change that still needs to be done.

Improve set_osd call.
Extend FTP setting support.
Add set_network_ntp
Add set_network_email
Fixes to add_user and modify_user.
GetAlarm is not supported on newer cameras.
Correct call is "SetEnc" not "SetRec".
Argument types changed.
Copy link
Member

@Benehiko Benehiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just some minor changes. The setup.py is a bit concerning, IMO it needs to be fixed versions.

Comment on lines +42 to +45
body = [{"cmd": "SetOsd", # "action": 1,
"param": {
"Osd": {
"bgcolor": bg_color,
# "bgcolor": bg_color,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are these commented out?

}}}]
# print("SetOsd:", json.dumps(body[0], indent=4))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just delete?

@@ -25,9 +25,127 @@ def set_net_port(self, http_port: float = 80, https_port: float = 443, media_por
"rtspPort": rtsp_port
}}}]
self._execute_command('SetNetPort', body, multi=True)
print("Successfully Set Network Ports")
# print("Successfully Set Network Ports")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# print("Successfully Set Network Ports")
print("Successfully Set Network Ports")

r_data = self._execute_command('ModifyUser', body)[0]
if r_data["value"]["rspCode"] == 200:
# print(f"modify user: {username}\nCamera responded with: {json.dumps(r_data, indent=4)}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# print(f"modify user: {username}\nCamera responded with: {json.dumps(r_data, indent=4)}")

body = [{"cmd": "Login", "action": 0,
"param": {"User": {"userName": self.username, "password": self.password}}}]
param = {"cmd": "Login", "token": "null"}
response = Request.post(self.url, data=body, params=param)
if response is not None:
# print("LOGIN GOT: ", response.text)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# print("LOGIN GOT: ", response.text)

@@ -64,28 +68,47 @@ def login(self) -> bool:
:return: bool
"""
try:
# Suppress only the single warning from urllib3 needed.
requests.packages.urllib3.disable_warnings(category=InsecureRequestWarning)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe make this optional?

or what about just leaving this to the client to set this as an env variable

PYTHONWARNINGS="ignore:Unverified HTTPS request"

Comment on lines +84 to +86
# print("Login success")
else:
# print(self.token)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the comments or restore the print of "Login success?"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants